Skip to content

Fixes to per-layer lr-scale#243

Merged
RaymondLi0 merged 10 commits into
mainfrom
raymond/per_layer_lr_scale
Jun 16, 2025
Merged

Fixes to per-layer lr-scale#243
RaymondLi0 merged 10 commits into
mainfrom
raymond/per_layer_lr_scale

Conversation

@RaymondLi0
Copy link
Copy Markdown
Contributor

@RaymondLi0 RaymondLi0 commented Apr 29, 2025

✨ Description

Specify different lr-scales per layer.

🔍 Type of change

Select all that apply:

  • 🐛 Bug fix (non-breaking change that addresses a specific issue)
  • 🚀 New feature (non-breaking change that adds functionality)
  • ⚠️ Breaking change (a change that could affect existing functionality)
  • 📈 Performance improvement/optimization (improves speed, memory usage, or efficiency)
  • 🛠️ Code refactor (non-functional changes that improve code readability, structure, etc.)
  • 📦 Dependency bump (updates dependencies, including Dockerfile or package changes)
  • 📝 Documentation change (updates documentation, including new content or typo fixes)
  • 🔧 Infrastructure/Build change (affects build process, CI/CD, or dependencies)

📝 Changes

List the key changes introduced in this PR:

  1. Change A
  2. Change B

✅ Checklist

Make sure the following tasks are completed before submitting the PR:

General

  • 📜 I have read and followed the contributing guidelines.
  • 🏷️ I am using a clear and descriptive PR title that summarizes the key change or feature introduced.
  • 🎉 The functionality is complete, and I have tested the changes.
  • 📝 I have updated the documentation if needed.
  • ⚠️ The change does not introduce any new issues (e.g., runtime warnings, type checker errors, linting problems, unhandled edge cases).
  • 🧩 I have commented my code, especially in hard-to-understand areas.

Dependencies and Configuration

  • 🐋 I have updated the Docker configuration or dependencies, if applicable.
  • 🔄 I have ensured compatibility with the existing setup after dependency changes.

Testing

  • 🧪 I have added or updated tests to cover my changes.
  • ✔️ New and existing tests pass locally with my changes.
  • 🚦 I have tested these changes on GPUs and verified training stability.
  • 🏋️ I have tested the changes on realistic training workloads, if applicable.

Performance Impact

  • 📊 I have run benchmarks where applicable to evaluate the performance impact.
  • ✅ The benchmarks show no performance regression.
  • 🚀 The benchmarks indicate a potential performance improvement.
  • ⚠️ The benchmarks indicate a potential performance degradation.
  • 📈 I have provided benchmark results and detailed any performance impact below, if applicable.

📊 Performance Impact Details

If there is any impact on performance, describe it and provide benchmark results, if applicable:


🗒️ Additional Notes

Include any additional context, information, or considerations here, such as known issues, follow-up tasks, or backward compatibility concerns.

Comment thread fast_llm/layers/transformer/config.py Outdated
@jlamypoirier jlamypoirier changed the title For visibility: add per-layer lr-scale [Prototype] For visibility: add per-layer lr-scale Jun 4, 2025
@oleksost
Copy link
Copy Markdown
Contributor

I think this can be closed as this has been merged in #258

@RaymondLi0 RaymondLi0 marked this pull request as ready for review June 16, 2025 17:31
@RaymondLi0
Copy link
Copy Markdown
Contributor Author

Sorry for the delay on this one. @oleksost could you have another look?
Seems some fixes were not included in #258

@RaymondLi0 RaymondLi0 changed the title [Prototype] For visibility: add per-layer lr-scale Fixes to per-layer lr-scale Jun 16, 2025
@oleksost oleksost self-requested a review June 16, 2025 18:53
@oleksost
Copy link
Copy Markdown
Contributor

Thanks for finding these bugs! These should be merged asap I think

@RaymondLi0 RaymondLi0 merged commit 1371e47 into main Jun 16, 2025
4 checks passed
@RaymondLi0 RaymondLi0 deleted the raymond/per_layer_lr_scale branch June 16, 2025 19:27
self._tensor_space,
# TODO MTP: which index?
layer_index=max(self._config.transformer.num_layers, 1),
layer_index=max(self._config.transformer.num_layers + i, 1),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have unintended consequences on the initialization scale.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only affects the prediction-heads for i>0 (thus not the next-token prediction)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the layer index is used elsewhere. It looks like it's only used in the backup attention regularization though, so it doesn't matter much https://github.com/ServiceNow/Fast-LLM/blob/main/fast_llm/layers/transformer/attention.py#L181. I got mixed up with num_layers which does matter for initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants